fix(sso): scope redirect loop counter to SSO hops#1302
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR refines SSO redirect-loop detection to scope the counter only to actual SSO loop hops. A new ChangesSSO loop detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
inc/sso/class-sso.php (1)
441-441: ⚡ Quick winUse Yoda comparisons in this production conditional.
Please switch the comparisons at Line 441 to Yoda style for consistency with repository standards.
Proposed fix
- if ($threshold < 1 || $window < 1 || ! $this->is_sso_loop_request()) { + if (1 > $threshold || 1 > $window || ! $this->is_sso_loop_request()) { return false; }As per coding guidelines: "Use Yoda conditions in production code (e.g., 'value' === $var, not $var === 'value')"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/sso/class-sso.php` at line 441, Change the conditional to use Yoda comparisons: replace "if ($threshold < 1 || $window < 1 || ! $this->is_sso_loop_request())" with a Yoda-style form such as "if (1 > $threshold || 1 > $window || false === $this->is_sso_loop_request())" so the constants come first; update this inside the same method in class-sso.php where $threshold, $window and the is_sso_loop_request() call are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/sso/class-sso.php`:
- Around line 491-494: The referer check is too permissive because it only
compares $referer_path to $sso_path; update the logic in class-sso.php so it
validates both host and a strict endpoint boundary: parse the referer host (e.g.
$referer_host = (string) wp_parse_url($referer, PHP_URL_HOST)) and compare it to
the site/home host, then ensure the path exactly matches $sso_path or starts
with $sso_path followed by '/' (to avoid matching paths like '/ssohijack'); use
$this->get_url_path(), $referer_path and wp_parse_url to implement these two
checks and only return true when both host and bounded-path conditions are met.
---
Nitpick comments:
In `@inc/sso/class-sso.php`:
- Line 441: Change the conditional to use Yoda comparisons: replace "if
($threshold < 1 || $window < 1 || ! $this->is_sso_loop_request())" with a
Yoda-style form such as "if (1 > $threshold || 1 > $window || false ===
$this->is_sso_loop_request())" so the constants come first; update this inside
the same method in class-sso.php where $threshold, $window and the
is_sso_loop_request() call are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: effcff6f-f8cb-4b91-a213-7c2ebce5c7ef
📒 Files selected for processing (2)
inc/sso/class-sso.phptests/WP_Ultimo/SSO/SSO_Test.php
Comment on lines
+491
to
+494
| $referer_path = (string) wp_parse_url($referer, PHP_URL_PATH); | ||
| $sso_path = '/' . ltrim($this->get_url_path(), '/'); | ||
|
|
||
| return 0 === strpos($referer_path, $sso_path); |
Contributor
There was a problem hiding this comment.
Tighten referer fingerprint to main-site /sso endpoint only.
Line 494 currently treats any referer path starting with /sso as an SSO hop, regardless of host and endpoint boundary. That can still consume wu_sso_redirect_attempts for non-SSO traffic.
Proposed fix
- $referer_path = (string) wp_parse_url($referer, PHP_URL_PATH);
- $sso_path = '/' . ltrim($this->get_url_path(), '/');
-
- return 0 === strpos($referer_path, $sso_path);
+ $referer_host = strtolower((string) wp_parse_url($referer, PHP_URL_HOST));
+ $referer_path = (string) wp_parse_url($referer, PHP_URL_PATH);
+ $main_host = strtolower((string) wp_parse_url(get_home_url(wu_get_main_site_id()), PHP_URL_HOST));
+ $sso_path = '/' . ltrim($this->get_url_path(), '/');
+
+ if (empty($referer_host) || $referer_host !== $main_host) {
+ return false;
+ }
+
+ return 1 === preg_match('#^' . preg_quote($sso_path, '#') . '(?:/|$)#', $referer_path);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/sso/class-sso.php` around lines 491 - 494, The referer check is too
permissive because it only compares $referer_path to $sso_path; update the logic
in class-sso.php so it validates both host and a strict endpoint boundary: parse
the referer host (e.g. $referer_host = (string) wp_parse_url($referer,
PHP_URL_HOST)) and compare it to the site/home host, then ensure the path
exactly matches $sso_path or starts with $sso_path followed by '/' (to avoid
matching paths like '/ssohijack'); use $this->get_url_path(), $referer_path and
wp_parse_url to implement these two checks and only return true when both host
and bounded-path conditions are met.
Collaborator
Author
Summary
Testing
Merged via PR #1302 to main. aidevops.sh v3.20.2 spent 38s on this as a headless bash routine. |
superdav42
added
the
review-feedback-scanned
This was referenced May 28, 2026
superdav42
added a commit
that referenced
this pull request
May 29, 2026
…1309) The unattached-broker JSONP branch in handle_broker() returned '{code: 0, message: "Broker not attached"}', which sso.js treats as a denial (assets/js/sso.js lines 101-117). That set the wu_sso_denied cookie for 5 minutes on the very first subsite front-end page-load, silently disabling auto-SSO before it could complete a single round-trip — even when the user was in fact logged in on the main site. The denial payload is the wrong signal here: the broker not being attached on the JSONP probe is the expected state on the first hit, not a failure. Return 'verify: must-redirect' instead so the already-existing sso.js branch (assets/js/sso.js lines 93-95) performs a top-level navigation to '<broker>/sso?return_url=...'. That request re-enters handle_broker() as a regular page load and falls through to the non-JSONP redirect, which after #1301 reaches the cookie-less server handler via getAttachUrl() and completes the SSO round-trip. This is the minimal subset of #1297 that the post-#1301/#1302 codebase still needs. The /sso-grant routing fix (#1301) and the scoped redirect-loop counter (#1302) already shipped, so the rest of #1297's PHP changes are no longer required. Regression tests in tests/WP_Ultimo/SSO/SSO_Test.php: - test_handle_broker_source_jsonp_unattached_returns_must_redirect: locks in the new payload and forbids the legacy 'Broker not attached' denial. - test_sso_js_handles_must_redirect_verify: confirms sso.js still recognises the must-redirect verify value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/ssoreferers.Testing
WP_TESTS_DIR=/tmp/wordpress-tests-lib vendor/bin/phpunit --filter SSO_Testvendor/bin/phpcs inc/sso/class-sso.php tests/WP_Ultimo/SSO/SSO_Test.php(passes with existing test warnings forfile_get_contents())MERGE_SUMMARY: Scoped
should_skip_redirect_due_to_loop()so only SSO-fingerprinted requests consume thewu_sso_redirect_attemptsbudget, with PHPUnit coverage for non-SSO requests and SSO referers.Resolves #1299
Summary by CodeRabbit
Bug Fixes
Tests